Make AutoTimestampEventListener temporary disabling thread-safe (8.0.x)#15954
Merged
jdaugherty merged 4 commits intoJul 10, 2026
Merged
Conversation
Fixes apache#14510. The withoutTimestamps, withoutDateCreated and withoutLastUpdated methods disabled timestamping by mutating the listener-wide entity maps (capturing each entry and setting it to Optional.empty()), which is JVM-global state. Overlapping windows on concurrent threads corrupted each other: a thread leaving its window re-enabled timestamping for every other thread still inside one, so historical dateCreated/lastUpdated values were silently overwritten. An exception inside the closure also left timestamping permanently disabled because the previous state was restored without try/finally. Suppression is now tracked per thread: the property-name getters consult a ThreadLocal holding an all-entities nesting depth plus the set of entity names disabled by per-class scopes. Each scope only re-enables the names it added, so nested and overlapping scopes restore correctly, and try/finally guarantees restoration when the closure throws. The shared metadata maps are no longer mutated. The CreatedBy/UpdatedBy auditing maps are untouched, as no withoutXxx API covers them. Behavioral note: disabling is now scoped to the calling thread; threads spawned inside the closure are no longer affected. The GORM auto-timestamping guide documents this. The spec now exercises the listener through beforeInsert/beforeUpdate and covers cross-thread isolation, overlapping windows, nested and overlapping same-class scopes, exception restoration and concurrent use. Forward-port of the 7.0.x fix (apache#15952) to 8.0.x, identical to the 7.1.x port (apache#15953), as this class is identical on 7.1.x through 8.0.x.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #15954 +/- ##
==================================================
+ Coverage 49.5558% 49.6071% +0.0514%
- Complexity 16930 16951 +21
==================================================
Files 1999 1999
Lines 93753 93791 +38
Branches 16419 16421 +2
==================================================
+ Hits 46460 46527 +67
+ Misses 40131 40092 -39
- Partials 7162 7172 +10
🚀 New features to boost your workflow:
|
Contributor
|
@codeconsole I pushed some improvements (there were missing docs, dead code, and multi-threading could be broken by your change so I documented how to adjust the code) |
borinquenkid
previously requested changes
Jul 10, 2026
borinquenkid
left a comment
borinquenkid
left a comment
Member
There was a problem hiding this comment.
the codecov flagged branches not covered by modifications. Can you execute them?
Two additions to the spec: - Registering auditor properties through persistentEntityAdded and asserting CreatedBy/LastModifiedBy population on insert and update, which executes the storeAuditorAvailability body. - Capturing a suppression snapshot inside a window opened with an empty class list, where the thread-local state is present but empty, which takes the isEmpty() branch of both TimestampSuppression constructor ternaries. Also pins that an empty class list disables nothing and that the resulting snapshot suppresses nothing.
Contributor
Author
|
@borinquenkid done in 43e59b4 — two tests added:
Verified locally with JaCoCo: both lines fully executed, 4/4 branches covered on both ternaries. Codecov should report 100% patch coverage on the next run. |
Contributor
Author
|
Thanks @jdaugherty — reviewed, LGTM. Good catch on the One semantic worth stating for the record: Ready to merge from my side. |
✅ All tests passed ✅🏷️ Commit: 43e59b4 Learn more about TestLens at testlens.app. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #14510 on 8.0.x. Identical to the 7.1.x port #15953 (this class is byte-identical on 7.1.x through 8.0.x — the cherry-pick applied with zero conflicts); see also the original 7.0.x fix #15952.
The bug
withoutTimestamps,withoutDateCreatedandwithoutLastUpdateddisabled timestamping by mutating the listener-wide entity maps (capturing each entry and setting it toOptional.empty(), then restoring). That state is JVM-global, so overlapping windows on concurrent threads corrupt each other:dateCreated/lastUpdatedvalues are silently clobberedReproduction: https://github.com/codeconsole/grails-autotimestamp-bug
A second facet: there was no
try/finally, so an exception inside the closure left timestamping permanently disabled for the whole JVM.The fix
The shared metadata maps are no longer mutated. Suppression is tracked per thread: the property-name getters consult a
ThreadLocalholding an all-entities nesting depth plus the set of entity names disabled by per-class scopes. Each scope only re-enables the names it added, so nested and overlapping scopes restore correctly;try/finallyguarantees restoration when the closure throws; theThreadLocalis removed when empty so nothing is retained on pooled threads.Public API, protected fields and single-thread semantics are unchanged (binary and source compatible). The
CreatedBy/UpdatedByauditing maps are untouched — nowithoutXxxAPI covers them.Merge coordination
Once this and #15953 are both merged, the routine 7.1.x → 7.2.x → 8.0.x cascade merges see identical content on both sides and resolve cleanly. The only conflicted hop is 7.0.x → 7.1.x after #15952, which resolves as keep-7.1.x's-listener.
Behavioral note
Disabling is now scoped to the calling thread. Threads spawned inside the closure are no longer affected — under the old global behavior they were (by accident). Code relying on that must call
withoutTimestampson each thread. The auto-timestamping guide now documents the thread scoping. Worth a mention in the release notes.Tests
The spec exercises the listener through the public
beforeInsert/beforeUpdateAPI, with tests that reproduce the bug (verified red on the unfixed listener on 7.0.x and 7.1.x, whose suppression code is identical to 8.0.x's):All 248 tests in
grails-datamapping-corepass on the 8.0.x toolchain (JDK 25/Groovy 5), as do the timestamp specs ingrails-datamapping-core-test,grails-data-hibernate5-coreandgrails-test-suite-uber; Checkstyle and CodeNarc are clean.